Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libtest: Don't panic if unable to spawn thread #78165

Closed
wants to merge 4 commits into from

Conversation

camelid
Copy link
Member

@camelid camelid commented Oct 20, 2020

Instead just run the test synchronously.

Instead just run the test synchronously.
@camelid camelid added the A-libtest Area: #[test] related label Oct 20, 2020
@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2020
@@ -470,7 +470,10 @@ pub fn run_test(
let supports_threads = !cfg!(target_os = "emscripten") && !cfg!(target_arch = "wasm32");
if concurrency == Concurrent::Yes && supports_threads {
let cfg = thread::Builder::new().name(name.as_slice().to_owned());
cfg.spawn(runtest).unwrap();
if cfg.spawn(runtest).is_err() {
// If we can't spawn a new thread, just run the test synchronously.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we print a message letting the user know?

@camelid
Copy link
Member Author

camelid commented Oct 20, 2020

I don't have a ton of experience with closures: what's the best way to fix this?

error[E0382]: use of moved value: `runtest`
   --> library/test/src/lib.rs:475:17
    |
473 |             if cfg.spawn(runtest).is_err() {
    |                          ------- value moved here
474 |                 // If we can't spawn a new thread, just run the test synchronously.
475 |                 runtest();
    |                 ^^^^^^^ value used here after move
    |
note: closure cannot be invoked more than once because it moves the variable `desc` out of its environment
   --> library/test/src/lib.rs:451:17
    |
451 |                 desc,
    |                 ^^^^

@jyn514
Copy link
Member

jyn514 commented Oct 20, 2020

@camelid there is not a simple way to fix it; run_test_in_process and spawn_test_subprocess both consume a TestDesc, but you don't know whether they will be called or not before spawn() fails. Actually that caught a different error: this should either assert the failure is WouldBlock or only call runtest() directly if the variant is WouldBlock.

The simplest way is probably to get run_test_in_process to take &mut TestDesc instead, but that might be tricky.

@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 20, 2020
@camelid
Copy link
Member Author

camelid commented Oct 20, 2020

Actually that caught a different error: this should either assert the failure is WouldBlock or only call runtest() directly if the variant is WouldBlock.

Should be fixed now.

library/test/src/lib.rs Outdated Show resolved Hide resolved
library/test/src/lib.rs Outdated Show resolved Hide resolved
library/test/src/lib.rs Outdated Show resolved Hide resolved
camelid and others added 2 commits October 20, 2020 15:55
Co-authored-by: Joshua Nelson <joshua@yottadb.com>
@jyn514
Copy link
Member

jyn514 commented Oct 22, 2020

@camelid can you remove 'may fix' from the commit message? This PR isn't actually fixing the same problem (although it might fix a related one).

@bors
Copy link
Contributor

bors commented Jan 26, 2021

☔ The latest upstream changes (presumably #81367) made this pull request unmergeable. Please resolve the merge conflicts.

@andersk
Copy link
Contributor

andersk commented Jan 26, 2021

There’s probably a better way, but one brute-force strategy that works without much thinking would be to encase the closure in an Arc<Mutex<Option<…>>>:

let runtest = Arc::new(Mutex::new(Some(move || match opts.strategy {})));
…
cfg.spawn({
    let runtest = Arc::clone(&runtest);
    move || runtest.lock().unwrap().take().unwrap()()
})
…
runtest.lock().unwrap().take().unwrap()()

@ghost
Copy link

ghost commented Jan 29, 2021

@camelid Any update? I would like to take over this if you run out of threads time.
EDIT: I think I have a working patch now: 97ca47f4708d85bf47b803377fc73ff39314c9f1.

There’s probably a better way, but one brute-force strategy that works without much thinking would be to encase the closure in an Arc<Mutex<Option<…>>>

I think Arc::get_mut and Mutex::get_mut could also be helpful.

@andersk
Copy link
Contributor

andersk commented Jan 29, 2021

Arc::get_mut and Mutex::get_mut require a unique reference and so can't be used from the thread.

@ghost
Copy link

ghost commented Jan 29, 2021

Arc::get_mut and Mutex::get_mut require a unique reference and so can't be used from the thread.

I meant thay can be used after encountering ErrorKind::WouldBlock.

@jyn514
Copy link
Member

jyn514 commented Jan 29, 2021

EDIT: I think I have a working patch now: 97ca47f.

@hyd-dev I think it's fine to open a new PR and mention this one.

@jyn514
Copy link
Member

jyn514 commented Jan 29, 2021

err actually this is waiting-on-review, not waiting-on-author, so I don't know if there's any point to a new PR.

@camelid
Copy link
Member Author

camelid commented Jan 29, 2021

err actually this is waiting-on-review, not waiting-on-author, so I don't know if there's any point to a new PR.

This change doesn't work, so a new PR still makes sense 🙃

The reason it hasn't been reviewed is (1) it doesn't work and (2) withoutboats is taking a break.

@ghost
Copy link

ghost commented Jan 30, 2021

New PR: #81546

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 18, 2021
… r=Mark-Simulacrum

[libtest] Run the test synchronously when hitting thread limit

libtest currently panics if it hits the thread limit. This often results in spurious test failures (<code>thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 11, kind: WouldBlock, message: "Resource temporarily unavailable" }'</code> ... `error: test failed, to rerun pass '--lib'`). This PR makes it continue to run the test synchronously if it runs out of threads.

Closes rust-lang#78165.

`@rustbot` label: A-libtest T-libs
@bors bors closed this in 55ab2e3 Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-libtest Area: #[test] related S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants